Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(spv): shader debug option #4028

Merged
merged 4 commits into from
Oct 23, 2023
Merged

feat(spv): shader debug option #4028

merged 4 commits into from
Oct 23, 2023

Conversation

wicast
Copy link
Contributor

@wicast wicast commented Aug 10, 2023 •
edited
Loading

Checklist

  • Run cargo clippy.
  • Run cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Connections
#3896

Description
Generate shader debug info for spv

Testing
There are some spv level tests in naga already. Testing with frame debugger is hard to make an autotest.

So the following step needs done by hands:
add debug flag to the shader macro wgpu::include_wgsl!("shader.wgsl", true)); or use struct

wgpu::ShaderModuleDescriptor {
        label: Some("shader.wgsl"),
        source: wgpu::ShaderSource::Wgsl(Cow::Borrowed(include_str!("shader.wgsl"))),
        debug: true,
    }

Make sure wgpu is using Vulkan backend.
Launch compiled exe from Renderdoc and capture a frame and trigger the shader debugger via vertex or pixel.
press F10 to step over and F10+shift to step back.
图片

Test on Win10 19045.3324, Nvidia.

I also tried other debugger like PIX, Nsight, but only Renderdoc support vulkan+spv debug.

I'll test Linux+Renderdoc later.

@wicast wicast requested a review from crowlKats as a code owner August 10, 2023 15:09
@wicast wicast force-pushed the shader_debug_flag branch from 0a4039c to 88e83c5 Compare August 10, 2023 15:47
Wumpf
Wumpf reviewed Aug 14, 2023
View reviewed changes
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that this flag only applies to spirv-out I'm wondering we could find another way to specify this instead of putting it on every ShaderModuleDescriptor

That said, maybe this is something we could expose for more inputs - @teoxoy can you comment on that? If we can make more use of that it's surely a useful pattern, otherwise I'm a bit on the fence, but still pondering what might be better there. We should definitely expose this somehow

wgpu-core/src/device/resource.rs Show resolved Hide resolved
@wicast
Copy link
Contributor Author

wicast commented Aug 15, 2023 •
edited
Loading

Given that this flag only applies to spirv-out I'm wondering we could find another way to specify this instead of putting it on every ShaderModuleDescriptor

That said, maybe this is something we could expose for more inputs - @teoxoy can you comment on that? If we can make more use of that it's surely a useful pattern, otherwise I'm a bit on the fence, but still pondering what might be better there. We should definitely expose this somehow

Honestly I have the same thought on it while writing, but I didn't want to changing the create_shader_module api too much at the time.

Now I have two idea:

  1. add another option struct for create_shader_module.
  2. add another api on device like set_shader_compile_option, every call on create_shader_module will query this option.

i vote for the second plan, because this will leave create_shader_module unchanged and you don't need to set a debug flag on every shader creation.

@cwfitzgerald
Copy link
Member

This kind of switch shows up in multiple places and is likely okay being a instance level switch for all shaders from the instance.

We have a couple things that we want to be able to adjust on the instance level, including turning on/off underlying-api validation, debug labels, and graphics debugger integrations. I'm going to make a instance flags in a PR shortly and this can use that.

@cwfitzgerald
Copy link
Member

Ping from triage

@cwfitzgerald cwfitzgerald requested a review from a team September 20, 2023 18:57
@teoxoy
Copy link
Member

teoxoy commented Oct 4, 2023

Sorry for taking a while to respond on this but I think what @cwfitzgerald has in mind sounds good to me.

@cwfitzgerald
Copy link
Member

To clarify, I will implement the flags in general very soon, then you can merge on top of that work

@jimblandy
Copy link
Member

Firefox wants this. We want to be able to enable the backend's debugging layers in non-debug Firefox builds, for fuzzing and things like that.

@cwfitzgerald
Copy link
Member

#2316 is the main issue for the flags

@nical nical mentioned this pull request Oct 11, 2023
Merged
2 tasks
@cwfitzgerald
Copy link
Member

#4230 has now landed, giving you a flag to latch onto!

@wicast
Copy link
Contributor Author

wicast commented Oct 12, 2023

#4230 has now landed, giving you a flag to latch onto!

sure! I will take some time to integrate

@nical
Copy link
Contributor

nical commented Oct 16, 2023

#4246 adds the plumbing to access the instance flags from the instance or the device.

crowlKats
crowlKats reviewed Oct 20, 2023
View reviewed changes
deno_webgpu/shader.rs Outdated Show resolved Hide resolved
@wicast wicast requested a review from a team October 20, 2023 14:07
@wicast wicast force-pushed the shader_debug_flag branch 3 times, most recently from dbe5c82 to b7de7fd Compare October 20, 2023 15:24
@wicast wicast force-pushed the shader_debug_flag branch from b7de7fd to e423f58 Compare October 21, 2023 02:52
@cwfitzgerald cwfitzgerald enabled auto-merge (squash) October 23, 2023 23:33
@cwfitzgerald cwfitzgerald disabled auto-merge October 23, 2023 23:33
@cwfitzgerald cwfitzgerald enabled auto-merge (squash) October 23, 2023 23:34
@cwfitzgerald cwfitzgerald merged commit 9dc5761 into gfx-rs:trunk Oct 23, 2023
23 checks passed
@teoxoy teoxoy mentioned this pull request Nov 2, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crowlKats crowlKats crowlKats left review comments

@Wumpf Wumpf Wumpf left review comments

@cwfitzgerald cwfitzgerald cwfitzgerald approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@wicast @cwfitzgerald @teoxoy @jimblandy @nical @Wumpf @crowlKats